feat(mcp): improve neighbourhood publishing with auto-cloning#730
Merged
feat(mcp): improve neighbourhood publishing with auto-cloning#730
Conversation
Three improvements to make MCP neighbourhood publishing agent-friendly:
1. **New tool: list_link_language_templates** - Returns available P2P
sync templates (e.g. Holochain perspective-diff-sync). Agents pick
from this list instead of manually looking up addresses.
2. **Auto-clone in neighbourhood_publish** - Agents pass a template
address + name. The tool auto-clones the template with a unique ID,
publishes the clone, and uses it for the neighbourhood. No manual
language cloning workflow needed.
3. **Strip implementation details from descriptions** - Tools now
describe what they do for agents, not how they're implemented.
Removed mentions of specific language addresses in favor of
higher-level concepts.
Workflow now:
templates = list_link_language_templates()
neighbourhood_publish_from_perspective(
perspective_uuid, templates[0], "My Neighbourhood"
)
Before, agents needed to understand link languages, cloning, and
address management. Now it's one step: pick template, publish.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new languages MCP tool module and registers language-related routes; implements listing and cloning of link-language templates, updates neighbourhood publish parameters/response to clone templates on publish, and adds related docs. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Handler as Ad4mMcpHandler
participant Runtime as RuntimeService
participant LangCtrl as LanguageController
rect rgba(100,200,100,0.5)
Note over Client,Runtime: List link-language templates
Client->>Handler: list_link_language_templates()
Handler->>Handler: Check RUNTIME_KNOWN_LINK_LANGUAGES_READ_CAPABILITY
Handler->>Runtime: Query known link language templates
Runtime-->>Handler: Return template list
Handler-->>Client: Return templates
end
rect rgba(100,150,200,0.5)
Note over Client,LangCtrl: Publish neighbourhood using template
Client->>Handler: neighbourhood_publish_from_perspective(template_addr, name)
Handler->>Handler: clone_link_language(template_addr, name)
Handler->>LangCtrl: Request clone -> create unique instance
LangCtrl-->>Handler: Return cloned_address
Handler->>Runtime: Save/load cloned instance
Handler->>Handler: Publish neighbourhood with cloned_address
Handler-->>Client: Return {cloned_link_language, name, ...}
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Run cargo fmt on all files - list_link_language_templates now returns address, name, description, author, and possible_template_params for each template instead of just opaque address strings - Falls back to local name lookup if language meta fetch fails
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rust-executor/src/mcp/tools/neighbourhoods.rs (1)
65-100: Filter results to ensure only template languages are returned.The function retrieves all addresses from
get_know_link_languages()without verifying they are actually templates. SinceLanguageMeta.templateddefaults toSome(false), non-template languages could be included in the result. Add a check to filter bytemplated == Some(true):Suggested fix
match meta { Ok(m) => { if m.templated == Some(true) { templates.push(json!({ "address": m.address, "name": m.name, "description": m.description, "author": m.author, "possible_template_params": m.possible_template_params, })); } } Err(_) => { ... } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-executor/src/mcp/tools/neighbourhoods.rs` around lines 65 - 100, The code currently collects all addresses from RuntimeService::with_global_instance(get_know_link_languages) and unconditionally pushes metadata into templates after calling LanguageController::get_language_expression; update the match handling in the loop so that when get_language_expression(address) returns Ok(m) you only push the entry into templates if m.templated == Some(true) (skip non-templated languages), preserving the existing Err fallback that uses LanguageController::get_language_name(address) for minimal info; reference LanguageController::global_instance, get_language_expression, get_language_name, LanguageMeta.templated and the templates Vec to locate where to apply the filter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust-executor/src/mcp/tools/neighbourhoods.rs`:
- Around line 175-183: The current publish path loads the cloned bundle using
controller.load_language but swallows errors and still returns Ok(address),
leaving the neighbourhood unusable; update the publish function to propagate the
load failure instead of returning Ok when
controller.load_language(bundle_on_disk, false).await returns Err (or at minimum
return a Result that indicates the load error), so callers like
neighbourhood_publish_from_perspective and
neighbourhood_publish_from_perspective_with_context receive the failure;
specifically, check the result of controller.load_language and convert/return
that error (including the address context) rather than only logging via
log::warn! when bundle_on_disk.exists() is true.
- Around line 154-183: The code saves the bundle with
controller.save_language_bundle but ignores its returned (hash, PathBuf), then
later constructs bundle_on_disk using the publish `address`, which may differ
and cause a miss; change the save call to capture the returned (hash, path)
(e.g., let (saved_hash, saved_path) =
controller.save_language_bundle(&input.bundle, None) or pattern-match the
Result) and use that saved hash or the returned PathBuf when checking/loading
the bundle (instead of using `address`), keeping the existing logging behavior
for save/load failures and leaving execute_on_language/publish_script unchanged.
---
Nitpick comments:
In `@rust-executor/src/mcp/tools/neighbourhoods.rs`:
- Around line 65-100: The code currently collects all addresses from
RuntimeService::with_global_instance(get_know_link_languages) and
unconditionally pushes metadata into templates after calling
LanguageController::get_language_expression; update the match handling in the
loop so that when get_language_expression(address) returns Ok(m) you only push
the entry into templates if m.templated == Some(true) (skip non-templated
languages), preserving the existing Err fallback that uses
LanguageController::get_language_name(address) for minimal info; reference
LanguageController::global_instance, get_language_expression, get_language_name,
LanguageMeta.templated and the templates Vec to locate where to apply the
filter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83b2ba93-e3e2-474e-87fb-009ce9a8edcb
📒 Files selected for processing (1)
rust-executor/src/mcp/tools/neighbourhoods.rs
- New language_meta tool returns full metadata for any language address: name, description, author, source_code_link, template params, etc. - Add unit tests for neighbourhood tools (param deserialization, template data uniqueness, response JSON structure) - Add unit tests for language_meta tool (param deserialization, response structure) - Update MCP skill docs with language/neighbourhood tool reference and workflow examples - cargo fmt applied
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
rust-executor/src/mcp/tools/neighbourhoods.rs (1)
154-183:⚠️ Potential issue | 🟠 MajorPath mismatch and silent load failure remain unaddressed.
The previous review identified two issues that still exist:
Path mismatch:
save_language_bundlereturns(hash, PathBuf)but the return value is ignored. Lines 176-178 constructbundle_on_diskusingaddressfrom the publish operation, which may differ from the local hash, causingbundle_on_disk.exists()to return false.Silent load failure: If
load_languagefails (line 180-182), the error is only logged and the function returnsOk(address). The language is published remotely but not loaded locally, which may causeneighbourhood_publish_from_perspectiveto fail downstream.🔧 Proposed fix addressing both issues
// Save locally - if let Err(e) = controller.save_language_bundle(&input.bundle, None) { - log::warn!("Failed to save cloned language bundle locally: {}", e); - } + let saved_bundle_path = match controller.save_language_bundle(&input.bundle, None) { + Ok((_hash, path)) => Some(path), + Err(e) => { + log::warn!("Failed to save cloned language bundle locally: {}", e); + None + } + }; // Publish via the language language // ... existing publish logic ... let address = address_raw.trim().trim_matches('"').to_string(); // Load into runtime - let bundle_on_disk = crate::utils::languages_directory() - .join(&address) - .join("bundle.js"); - if bundle_on_disk.exists() { - if let Err(e) = controller.load_language(bundle_on_disk, false).await { - log::warn!("Failed to load cloned language into runtime: {}", e); - } + if let Some(bundle_path) = saved_bundle_path { + controller.load_language(bundle_path, false).await + .map_err(|e| format!("Language published as '{}' but failed to load locally: {}", address, e))?; + } else { + return Err(format!( + "Language published as '{}' but bundle not saved locally for loading", + address + )); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-executor/src/mcp/tools/neighbourhoods.rs` around lines 154 - 183, The code ignores the (hash, PathBuf) returned by controller.save_language_bundle and therefore builds bundle_on_disk from the remote publish address which can mismatch, and it silently logs load_language failures instead of surfacing them; fix by capturing the return value of controller.save_language_bundle (the hash and saved_path), use the saved PathBuf to construct bundle_on_disk (instead of deriving from address), and if controller.load_language(saved_path, false).await returns Err propagate that error (convert to the existing error string format used elsewhere, e.g., map_err(|e| format!("Failed to load cloned language into runtime: {}", e))?) so the function returns an Err rather than logging and continuing, ensuring neighbourhood_publish_from_perspective sees the failure; keep the execute_on_language/publish flow unchanged except for using the saved path for local load.
🧹 Nitpick comments (2)
skills/ad4m/references/mcp.md (1)
60-67: Add language specifiers to fenced code blocks.The code blocks showing workflow examples are missing language specifiers. Per markdownlint MD040, fenced code blocks should have a language specified for proper syntax highlighting.
📝 Proposed fix
-``` +```text 1. list_link_language_templates → get available sync engines 2. neighbourhood_publish_from_perspective( perspective_uuid, link_language_template: templates[0].address, name: "My Neighbourhood" ) → auto-clones template, publishes, returns URL```diff -``` +```text neighbourhood_join_from_url(url: "neighbourhood://Qm...") → creates local perspective synced with the neighbourhood</details> Also applies to: 73-76 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@skills/ad4m/references/mcp.mdaround lines 60 - 67, The fenced code blocks
in this doc lack language specifiers; update each relevant fence (including the
examples containing list_link_language_templates,
neighbourhood_publish_from_perspective, and neighbourhood_join_from_url) to
include a language tag such as "text" (or another appropriate language)
immediately after the opening ``` so Markdown lint MD040 is satisfied and syntax
highlighting is applied consistently for both the block at the
list_link_language_templates/neighbourhood_publish_from_perspective example and
the block showing neighbourhood_join_from_url (also apply the same change to the
other block around lines 73–76).</details> </blockquote></details> <details> <summary>rust-executor/src/mcp/tools/languages.rs (1)</summary><blockquote> `42-51`: **Consider including the `templated` field in the response.** The `LanguageMeta` struct (per `graphql_types.rs:223-240`) includes a `templated: Option<bool>` field that indicates whether a language is a template. This field is omitted from the JSON response, which could be useful for agents to know whether they're inspecting a template or an instantiated language. <details> <summary>📝 Proposed fix</summary> ```diff let result = json!({ "address": meta.address, "name": meta.name, "description": meta.description, "author": meta.author, "possible_template_params": meta.possible_template_params, "source_code_link": meta.source_code_link, "template_applied_params": meta.template_applied_params, "template_source_language_address": meta.template_source_language_address, + "templated": meta.templated, }); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@rust-executor/src/mcp/tools/languages.rs` around lines 42 - 51, The JSON response built in the languages handler omits the templated flag from LanguageMeta; update the construction of the `result` JSON (where `meta` is used to populate fields) to include the `templated` field (i.e., use `meta.templated`) so the response returns whether the language is a template; locate the `result = json!({ ... })` block in this file and add `"templated": meta.templated` alongside the other properties so agents can see the template status. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In@rust-executor/src/mcp/tools/neighbourhoods.rs:
- Around line 154-183: The code ignores the (hash, PathBuf) returned by
controller.save_language_bundle and therefore builds bundle_on_disk from the
remote publish address which can mismatch, and it silently logs load_language
failures instead of surfacing them; fix by capturing the return value of
controller.save_language_bundle (the hash and saved_path), use the saved PathBuf
to construct bundle_on_disk (instead of deriving from address), and if
controller.load_language(saved_path, false).await returns Err propagate that
error (convert to the existing error string format used elsewhere, e.g.,
map_err(|e| format!("Failed to load cloned language into runtime: {}", e))?) so
the function returns an Err rather than logging and continuing, ensuring
neighbourhood_publish_from_perspective sees the failure; keep the
execute_on_language/publish flow unchanged except for using the saved path for
local load.
Nitpick comments:
In@rust-executor/src/mcp/tools/languages.rs:
- Around line 42-51: The JSON response built in the languages handler omits the
templated flag from LanguageMeta; update the construction of theresultJSON
(wheremetais used to populate fields) to include thetemplatedfield
(i.e., usemeta.templated) so the response returns whether the language is a
template; locate theresult = json!({ ... })block in this file and add
"templated": meta.templatedalongside the other properties so agents can see
the template status.In
@skills/ad4m/references/mcp.md:
- Around line 60-67: The fenced code blocks in this doc lack language
specifiers; update each relevant fence (including the examples containing
list_link_language_templates, neighbourhood_publish_from_perspective, and
neighbourhood_join_from_url) to include a language tag such as "text" (or
another appropriate language) immediately after the opening ``` so Markdown lint
MD040 is satisfied and syntax highlighting is applied consistently for both the
block at the list_link_language_templates/neighbourhood_publish_from_perspective
example and the block showing neighbourhood_join_from_url (also apply the same
change to the other block around lines 73–76).</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `0124dde7-6df3-4dc1-aa1c-43b9bbd1553b` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 68ea3bf8b8050ebc8184c905f1fc4544ecdb4b13 and 6103a54eaa6d15c0ecbdab0e633fd29cc520609a. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `rust-executor/src/mcp/tools/languages.rs` * `rust-executor/src/mcp/tools/mod.rs` * `rust-executor/src/mcp/tools/neighbourhoods.rs` * `skills/ad4m/references/mcp.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
lucksus
reviewed
Mar 7, 2026
Add #[serde(alias = "link_language")] to link_language_template field so existing tests and clients using the old parameter name continue to work. The new name is preferred but the old name is still accepted. All 7 tests pass: - 5 neighbourhood tests - 2 language tests
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust-executor/src/mcp/tools/neighbourhoods.rs (1)
27-40:⚠️ Potential issue | 🟠 MajorNew required
namefield is a breaking change.The
link_language→link_language_templaterename is handled gracefully via#[serde(alias = "link_language")], but the new requirednamefield has no default value. Existing clients that don't providenamewill fail deserialization. The test attests/js/tests/mcp-neighbourhood.test.ts:140-143calls this tool without thenamefield and will fail.Consider adding a default value:
💡 Suggested change
/// Human-readable name for this neighbourhood (used as the cloned language name) + #[serde(default = "default_neighbourhood_name")] pub name: String, } + +fn default_neighbourhood_name() -> String { + format!("Neighbourhood-{}", &uuid::Uuid::new_v4().to_string()[..8]) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-executor/src/mcp/tools/neighbourhoods.rs` around lines 27 - 40, The new required field `name` on NeighbourhoodPublishParams breaks existing clients; modify NeighbourhoodPublishParams so `name` has a serde default (e.g., add #[serde(default = "default_neighbourhood_name")] on the `name` field and add a small helper function like fn default_neighbourhood_name() -> String { "neighbourhood".into() }) or implement Default for NeighbourhoodPublishParams and use #[serde(default)] on the struct; this ensures deserialization succeeds when clients omit `name` while preserving the field in code that provides it.
♻️ Duplicate comments (2)
rust-executor/src/mcp/tools/neighbourhoods.rs (2)
155-184:⚠️ Potential issue | 🟠 MajorPath mismatch between save and load operations persists.
Per context snippet 3,
save_language_bundlereturns(hash, PathBuf)where the hash is calculated from the bundle content. This return value is ignored at line 156. Later (lines 177-179),bundle_on_diskis constructed usingaddressfrom the publish operation. If the publish address differs from the locally calculated hash,bundle_on_disk.exists()will return false, and the language won't be loaded.🔧 Proposed fix
- // Save locally - if let Err(e) = controller.save_language_bundle(&input.bundle, None) { - log::warn!("Failed to save cloned language bundle locally: {}", e); - } + // Save locally and capture the bundle path + let saved_bundle_path = match controller.save_language_bundle(&input.bundle, None) { + Ok((_hash, path)) => Some(path), + Err(e) => { + log::warn!("Failed to save cloned language bundle locally: {}", e); + None + } + }; // ... publish logic (lines 160-174 unchanged) ... - // Load into runtime - let bundle_on_disk = crate::utils::languages_directory() - .join(&address) - .join("bundle.js"); - if bundle_on_disk.exists() { - if let Err(e) = controller.load_language(bundle_on_disk, false).await { + // Load into runtime using the saved bundle path + if let Some(bundle_path) = saved_bundle_path { + if let Err(e) = controller.load_language(bundle_path, false).await { log::warn!("Failed to load cloned language into runtime: {}", e); } },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-executor/src/mcp/tools/neighbourhoods.rs` around lines 155 - 184, The save_language_bundle return value (hash, PathBuf) from controller.save_language_bundle is ignored causing a path mismatch with the publish address; update the code around controller.save_language_bundle to capture its return (the calculated hash and path), and then use the returned PathBuf (or derive the load path from that returned hash) when constructing bundle_on_disk instead of using the publish `address` variable from execute_on_language; alternatively, after obtaining the publish `address`, ensure you move/rename the saved bundle to the publish-derived path before calling controller.load_language, so load_language is always given the actual on-disk bundle path returned by save_language_bundle.
176-184:⚠️ Potential issue | 🟠 MajorLoad failure after successful publish leaves neighbourhood in an inconsistent state.
If
load_languagefails (line 181-182), the function still returnsOk(address). The language is published remotely but not loaded locally. Whenneighbourhood_publish_from_perspectivesubsequently uses this address, operations may fail because the language isn't available in the local runtime.🔧 Proposed fix - propagate load failure
// Load into runtime let bundle_on_disk = crate::utils::languages_directory() .join(&address) .join("bundle.js"); - if bundle_on_disk.exists() { - if let Err(e) = controller.load_language(bundle_on_disk, false).await { - log::warn!("Failed to load cloned language into runtime: {}", e); - } + if !bundle_on_disk.exists() { + return Err(format!( + "Language published as '{}' but bundle not found locally at {:?}", + address, bundle_on_disk + )); + } + controller.load_language(bundle_on_disk, false).await + .map_err(|e| format!("Language published but failed to load locally: {}", e))?;,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-executor/src/mcp/tools/neighbourhoods.rs` around lines 176 - 184, The code logs and ignores errors from controller.load_language when loading bundle_on_disk, causing publish to return Ok(address) even though the runtime load failed; update neighbourhood_publish_from_perspective (or the surrounding function that returns Ok(address)) to propagate the load error instead of swallowing it: when controller.load_language(bundle_on_disk, false).await returns Err(e) return or map that error into the function's Result (preserving context like "failed to load cloned language for {address}") so the caller does not get Ok(address) when the local runtime load failed. Ensure you reference controller.load_language, bundle_on_disk and address when constructing the propagated error.
🧹 Nitpick comments (1)
rust-executor/src/mcp/tools/neighbourhoods.rs (1)
306-320: Add test for backward compatibility via serde alias.The
#[serde(alias = "link_language")]attribute enables backward compatibility, but there's no test verifying that the old field name still works. Consider adding a test:🧪 Suggested test
#[test] fn test_publish_params_backward_compat_link_language_alias() { // Old clients use "link_language" instead of "link_language_template" let json = r#"{ "perspective_uuid": "abc-123", "link_language": "QmzSYwdnDDbtgry2DmiqYpVtoPP75MabguWZUy1Ene7NzQAQWz2", "name": "My SoA Memory" }"#; let params: NeighbourhoodPublishParams = serde_json::from_str(json).unwrap(); assert_eq!( params.link_language_template, "QmzSYwdnDDbtgry2DmiqYpVtoPP75MabguWZUy1Ene7NzQAQWz2" ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-executor/src/mcp/tools/neighbourhoods.rs` around lines 306 - 320, Add a unit test to verify the serde alias for backward compatibility: create a test (e.g., test_publish_params_backward_compat_link_language_alias) that deserializes JSON using the old field name "link_language" into NeighbourhoodPublishParams and asserts that the struct's link_language_template field equals the expected value; this ensures the #[serde(alias = "link_language")] on NeighbourhoodPublishParams continues to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@rust-executor/src/mcp/tools/neighbourhoods.rs`:
- Around line 27-40: The new required field `name` on NeighbourhoodPublishParams
breaks existing clients; modify NeighbourhoodPublishParams so `name` has a serde
default (e.g., add #[serde(default = "default_neighbourhood_name")] on the
`name` field and add a small helper function like fn
default_neighbourhood_name() -> String { "neighbourhood".into() }) or implement
Default for NeighbourhoodPublishParams and use #[serde(default)] on the struct;
this ensures deserialization succeeds when clients omit `name` while preserving
the field in code that provides it.
---
Duplicate comments:
In `@rust-executor/src/mcp/tools/neighbourhoods.rs`:
- Around line 155-184: The save_language_bundle return value (hash, PathBuf)
from controller.save_language_bundle is ignored causing a path mismatch with the
publish address; update the code around controller.save_language_bundle to
capture its return (the calculated hash and path), and then use the returned
PathBuf (or derive the load path from that returned hash) when constructing
bundle_on_disk instead of using the publish `address` variable from
execute_on_language; alternatively, after obtaining the publish `address`,
ensure you move/rename the saved bundle to the publish-derived path before
calling controller.load_language, so load_language is always given the actual
on-disk bundle path returned by save_language_bundle.
- Around line 176-184: The code logs and ignores errors from
controller.load_language when loading bundle_on_disk, causing publish to return
Ok(address) even though the runtime load failed; update
neighbourhood_publish_from_perspective (or the surrounding function that returns
Ok(address)) to propagate the load error instead of swallowing it: when
controller.load_language(bundle_on_disk, false).await returns Err(e) return or
map that error into the function's Result (preserving context like "failed to
load cloned language for {address}") so the caller does not get Ok(address) when
the local runtime load failed. Ensure you reference controller.load_language,
bundle_on_disk and address when constructing the propagated error.
---
Nitpick comments:
In `@rust-executor/src/mcp/tools/neighbourhoods.rs`:
- Around line 306-320: Add a unit test to verify the serde alias for backward
compatibility: create a test (e.g.,
test_publish_params_backward_compat_link_language_alias) that deserializes JSON
using the old field name "link_language" into NeighbourhoodPublishParams and
asserts that the struct's link_language_template field equals the expected
value; this ensures the #[serde(alias = "link_language")] on
NeighbourhoodPublishParams continues to work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f16b817c-0c38-4abf-b7ba-236b9cc8cdad
📒 Files selected for processing (1)
rust-executor/src/mcp/tools/neighbourhoods.rs
The removed tests were just checking serde/json! macros work, not actual functionality. Integration tests in tests/js/tests/mcp-neighbourhood.test.ts already cover real MCP endpoint behavior. Added comments pointing to the integration tests.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
rust-executor/src/mcp/tools/neighbourhoods.rs (1)
155-183:⚠️ Potential issue | 🟠 MajorUse the saved bundle path and surface local load failures.
save_language_bundle()already gives you the exact on-disk path, but this code discards it, saves no metadata, and then guesses<languages_directory>/<published_address>/bundle.js. If that guess misses, orload_language()fails,clone_link_language()still returns success and the next step has to rely on remote propagation instead of the local artifact it just created.Suggested diff
- // Save locally - if let Err(e) = controller.save_language_bundle(&input.bundle, None) { - log::warn!("Failed to save cloned language bundle locally: {}", e); - } + // Save locally and keep the exact path for runtime loading + let saved_bundle_path = match controller.save_language_bundle(&input.bundle, Some(&input.meta)) { + Ok((_hash, path)) => Some(path), + Err(e) => { + log::warn!("Failed to save cloned language bundle locally: {}", e); + None + } + }; @@ - // Load into runtime - let bundle_on_disk = crate::utils::languages_directory() - .join(&address) - .join("bundle.js"); - if bundle_on_disk.exists() { - if let Err(e) = controller.load_language(bundle_on_disk, false).await { - log::warn!("Failed to load cloned language into runtime: {}", e); - } - } + // Load into runtime from the exact saved location + if let Some(bundle_path) = saved_bundle_path { + controller + .load_language(bundle_path, false) + .await + .map_err(|e| format!("Cloned language published as '{}' but failed to load locally: {}", address, e))?; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-executor/src/mcp/tools/neighbourhoods.rs` around lines 155 - 183, The code currently discards the path returned by controller.save_language_bundle and guesses bundle_on_disk; change clone_link_language to capture the returned on-disk path from controller.save_language_bundle(&input.bundle, None), persist any necessary metadata (e.g., published address), use that exact returned path when calling controller.load_language(...) instead of constructing languages_directory/.../bundle.js, and propagate/return an Err (instead of only logging) if controller.load_language(...) fails so clone_link_language does not report success when the local load fails; reference controller.save_language_bundle, controller.load_language, and the bundle path variable (bundle_on_disk/returned path) when making these changes.
🧹 Nitpick comments (1)
rust-executor/src/mcp/tools/neighbourhoods.rs (1)
91-98: Keeptemplates[]schema-stable on fallback.The success path includes
authorandpossible_template_params, but the fallback path drops both keys entirely. Returning them asnullkeeps the tool contract consistent for clients that parse the JSON structurally.Suggested diff
Err(_) => { // Fallback: try to get at least the name from local runtime let name = controller.get_language_name(address).await; templates.push(json!({ "address": address, "name": name, "description": null, + "author": null, + "possible_template_params": null, })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-executor/src/mcp/tools/neighbourhoods.rs` around lines 91 - 98, The fallback branch that builds the templates entry (inside the Err(_) arm) omits the author and possible_template_params keys, breaking the JSON schema; update the templates.push call in the Err(_) block (where controller.get_language_name(address).await is used) to include "author": null and "possible_template_params": null (or empty array if preferred) alongside "address", "name", and "description" so the returned object matches the success-path schema.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@rust-executor/src/mcp/tools/neighbourhoods.rs`:
- Around line 155-183: The code currently discards the path returned by
controller.save_language_bundle and guesses bundle_on_disk; change
clone_link_language to capture the returned on-disk path from
controller.save_language_bundle(&input.bundle, None), persist any necessary
metadata (e.g., published address), use that exact returned path when calling
controller.load_language(...) instead of constructing
languages_directory/.../bundle.js, and propagate/return an Err (instead of only
logging) if controller.load_language(...) fails so clone_link_language does not
report success when the local load fails; reference
controller.save_language_bundle, controller.load_language, and the bundle path
variable (bundle_on_disk/returned path) when making these changes.
---
Nitpick comments:
In `@rust-executor/src/mcp/tools/neighbourhoods.rs`:
- Around line 91-98: The fallback branch that builds the templates entry (inside
the Err(_) arm) omits the author and possible_template_params keys, breaking the
JSON schema; update the templates.push call in the Err(_) block (where
controller.get_language_name(address).await is used) to include "author": null
and "possible_template_params": null (or empty array if preferred) alongside
"address", "name", and "description" so the returned object matches the
success-path schema.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5ea6ad7-9535-4dab-942f-8196deec72ba
📒 Files selected for processing (2)
rust-executor/src/mcp/tools/languages.rsrust-executor/src/mcp/tools/neighbourhoods.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rust-executor/src/mcp/tools/languages.rs
1. Path mismatch: Now using returned path from save_language_bundle instead of reconstructing it manually. Also verify saved hash matches published address with warning if different. 2. Load failure handling: Return error instead of just logging warning. If language cannot be loaded locally, the neighbourhood would be unusable, so we fail fast with clear error message.
… schema Addresses CodeRabbit review comment about schema consistency. The fallback path was missing these fields that the success path includes.
1. Renamed field back to (with as alias) - This ensures schema shows as expected by tests 2. Made parameter optional with default Neighbourhood - Tests don't pass name, so it needs a default value This maintains full backward compatibility with existing API consumers g while still supporting the new auto-clone functionality.
lucksus
approved these changes
Mar 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
AI agents (including me when building the SoA prototype today) bypass MCP and fall back to GraphQL because the neighbourhood publishing flow requires too much low-level knowledge:
This defeats the purpose of MCP — it should be the only interface agents need.
Solution
Three improvements:
1.
list_link_language_templates— new toolReturns available P2P sync engine templates (currently just Holochain perspective-diff-sync). Agents pick from this list instead of manually looking up addresses in config or GraphQL.
2. Auto-clone in
neighbourhood_publish_from_perspectiveAgents now pass:
perspective_uuid— what to sharelink_language_template— which sync engine (fromlist_link_language_templates)name— human-readable neighbourhood nameThe tool handles:
No manual language management. One-step workflow.
3. Strip implementation details from descriptions
Tool descriptions now explain what ("publish a perspective as a neighbourhood") not how ("requires a perspective-diff-sync language address QmzSYwdn...").
Agents work at a higher level of abstraction.
Before/After
Before:
After:
Testing
Verified with:
cargo check— compiles cleanNext: test with actual neighbourhood creation (will do with proper SoA memory perspective).
Related
Part of making MCP the canonical AI-agent interface. Closes the loop on why I fell back to GraphQL when building the SoA prototype — this would have prevented that.
Summary by CodeRabbit
New Features
Improvements
Documentation